Implement guidellm UI #169
Conversation
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
markurtz
requested review from
Copilot and
markurtz
and removed request for
Copilot
|
📦 Build Artifacts Available |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the GuideLLM UI by wiring in React components, injecting window-sourced data scripts, and updating environment, asset, and testing configurations.
- Integrate new UI components (PageHeader, WorkloadDetails, MetricsSummary, WorkloadMetrics) into the main page layout
- Inject
runInfoScript,workloadDetailsScript, andbenchmarksScriptinto the root layout’s<head> - Update
.envfiles, package.json scripts/dependencies, asset imports, and testing setup for the UI
Reviewed Changes
Copilot reviewed 207 out of 207 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/app/page.tsx | Replaced placeholder Typography with real UI components |
| src/ui/app/not-found.tsx | Added custom 404 page |
| src/ui/app/layout.tsx | Inject window-data scripts into <head> and update favicon format |
| src/ui/app/assets/icons/index.tsx | Imported new PNG icons and reordered exports |
| src/ui/.env.staging, .env.production, .env.local, .env.example, .env.development | Added environment variable templates |
| src/guidellm/benchmark/benchmark.py | Inlined list comprehension for iter_counts |
| package.json | Added "type": "module", updated scripts, dependencies, optionalDependencies |
| jest.setup.ts | Polyfilled fetch and mocked Nivo modules |
| jest.config.cjs | Disabled default coverage, adjusted reporters |
| eslint.config.js | Introduced new Flat ESLint configuration |
| cypress.config.ts | Minor comment adjustment on baseUrl |
| README.md | Documented GuideLLM UI usage and development notes |
| DEVELOPING.md | Added install instructions and test tagging guidelines |
| .prettierignore | Updated ignore rule for ESLint config |
| .husky/pre-commit | Enabled lint-staged pre-commit hook |
| .eslintrc.json | Removed outdated ESLint config |
Comments suppressed due to low confidence (5)
src/ui/app/assets/icons/index.tsx:5
- [nitpick] Icon exports should follow PascalCase (e.g.,
GuideLLMIconDark) to remain consistent with other component names.
import guideLLMIconDark from './guidellm-icon-dark.png';
jest.setup.ts:4
- Mocks for newly added Nivo modules (
@nivo/scales,@nivo/tooltip) should be added here to prevent import errors during tests.
jest.mock('@nivo/bar');
jest.config.cjs:9
- Coverage collection is disabled by default; consider enabling
collectCoverageto ensure UI test coverage metrics are maintained.
collectCoverage: false,
README.md:172
- [nitpick] Move 'This option is useful if:' to its own line under the heading to improve markdown readability.
2. Build and Serve the UI Locally (For Development) This option is useful if:
DEVELOPING.md:258
- Swap link text and URL for proper Markdown syntax:
[jest-runner-groups](https://www.npmjs.com/package/jest-runner-groups).
Reference [https://www.npmjs.com/package/jest-runner-groups](jest-runner-groups) Add @group with the tag in a docblock at the top of the test file to indicate which types of tests are contained within. Can't distinguish between different types of tests in the same file.
src/ui/app/layout.tsx
Outdated
| {/* <script | ||
| dangerouslySetInnerHTML={{ | ||
| __html: 'window.run_info = {}; window.workload_details = {}; window.benchmarks = {};', | ||
| }} | ||
| /> */} |
There was a problem hiding this comment.
[nitpick] Remove this commented-out initialization script block if it's no longer needed to keep the code clean.
| {/* <script | |
| dangerouslySetInnerHTML={{ | |
| __html: 'window.run_info = {}; window.workload_details = {}; window.benchmarks = {};', | |
| }} | |
| /> */} | |
| // Removed the commented-out script block for code cleanliness. |
Copilot uses AI. Check for mistakes.
src/guidellm/benchmark/benchmark.py
Outdated
| req.output_tokens | ||
| for req in total_with_output_first | ||
| ], | ||
| iter_counts=[req.output_tokens for req in total_with_output_first], |
There was a problem hiding this comment.
[nitpick] Consider extracting this comprehension into a named variable or formatting it across multiple lines to improve readability for long statements.
Copilot uses AI. Check for mistakes.
This PR actually implements the application logic including the UI/charts and the api setup that retrieves data from the window object.